Extend `QueryStability` to handle `IntoIterator` implementations #139345

smoelius · 2025-04-04T01:48:59Z

This PR extends the rustc::potential_query_instability lint to check values passed as IntoIterator implementations.

Full disclosure: I want the lint to warn about this line (please see #138871 for why):

Line 261 in aa8f0fd

.chain(&self.cache.external_paths)

However, the lint warns about several other lines as well.

Final note: the functions get_callee_generic_args_and_args and get_input_traits_and_projections were copied directly from Clippy's source code.

rustbot · 2025-04-04T01:49:03Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

smoelius · 2025-04-04T10:35:45Z

Apologies, I was not aware of the tests/ui-fulldeps/internal-lints/query_stability.rs test. I will refactor my changes to eliminate the duplicate warning.

But in your opinion, @fmease, should I keep the tests/ui/internal-lints/query_stability_into_iter.rs test?

fmease · 2025-04-04T10:54:11Z

I think it would make sense to merge tests/ui/internal-lints/query_stability_into_iter.rs into the preexisting tests/ui-fulldeps/internal-lints/query_stability.rs (and make your new test case use FxHash{Map,Set}).

Note that tests/ui-fulldeps/ tests require a stage2 compiler which takes extra compute power and time to build locally. Just as a heads up

smoelius · 2025-04-04T21:28:05Z

I think this is good to go, @fmease. Thank you for your suggestions.

EDIT: Nits are welcome, BTW.

smoelius · 2025-04-30T11:52:00Z

dfec3c0 is a small simplification.

@fmease Please let me know if you have any questions, or if I can do anything to make this PR easier to review.

smoelius · 2025-05-27T09:27:14Z

@fmease Are there any questions I could answer about this PR?

fmease

Sorry for the delay. I've had an initial scan. Not all of my comments are actionable unfortunately, I'll try to come back to it later.

compiler/rustc_interface/src/interface.rs

compiler/rustc_lint/src/internal.rs

fmease · 2025-05-27T12:03:07Z

compiler/rustc_lint/src/internal.rs

+    let Some(into_iterator_def_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) else {
+        return;
+    };
+    if expr.span.from_expansion() {


This suppresses the lint from firing for code like this?

fn iter<T>(x: impl IntoIterator<Item = T>) = impl Iterator<Item = T> { x.into_iter() } macro_rules! iter { ($e:expr) => { iter($e) } } fn take(map: std::collections::HashMap<i32, i32>) { _ = iter!(map); }

I think we should fire regardless. Internal lints can be a lot more aggressive than Clippy lints. There's a reason why rustc::potential_query_instability is marked report_in_external_macro: true.

Without that condition, an additional warning is produced here:

rust/tests/ui-fulldeps/internal-lints/query_stability.rs

Lines 22 to 23 in dfec3c0

for _ in x {}

//~^ ERROR using `into_iter`

+ error: using `into_iter` can result in unstable query results + --> $DIR/query_stability.rs:22:14 + | + LL | for _ in x {} + | ^ + | + = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

Not linting expanded code seemed like the most straightforward way of avoiding the duplicate warnings.

Would you prefer that the lint check the context in which the expression appears, e.g., something along these lines? https://doc.rust-lang.org/beta/nightly-rustc/src/clippy_utils/higher.rs.html#34-54

I think we should lint in expansions (generally), however, we could avoid looking for IntoITerator trait builds in case the lint already fired for the method call itself. i.e. if wee lint because we resolved to a method with the attribute, don't check whether that method also has a relevant IntoIterator bound

also, i think we can merge this lint into one

if let Some((callee_def_id, span, generic_args, _recv, _args)) = get_callee_span_generic_args_and_args(cx, expr)

first checking whether the method/call itself is marked as query unstable, then check whether it has a where-bound which relies on an unstable trait impl

Sorry, I'm not following. What do you mean by "merge this lint into one"?

rn u lint in two cases

the expr is a method/call

we can resolve it to a method with a #[rustc_lint_query_instability] attribute

the expr is a method

the method has a where-bound which is proven by an impl with a #[rustc_lint_query_instability] attribute.

Why not do

the expr is a method/call

we can resolve it to a method with a #[rustc_lint_query_instability] attribute

the method has a where-bound which is proven by an impl with a #[rustc_lint_query_instability] attribute.

Please tell me if what I pushed is what you had in mind.

tests/ui-fulldeps/internal-lints/query_stability.rs

compiler/rustc_lint/src/internal.rs

fmease · 2025-05-27T12:30:39Z

compiler/rustc_lint/src/internal.rs

-        if let Ok(Some(instance)) = ty::Instance::try_resolve(cx.tcx, cx.typing_env(), def_id, args)
+impl<'tcx> LateLintPass<'tcx> for QueryStability {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if let Some((span, def_id, args)) = typeck_results_of_method_fn(cx, expr)


It would be awesome if we could somehow unify typeck_results_of_method_fn and get_callee_generic_args_and_args.

Done in commit ae437dd.

compiler/rustc_lint/src/internal.rs

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

smoelius · 2025-05-28T22:22:02Z

Apologies. The force push was to remove an unnecessary change I included accidentally.

EDIT: Note that another commit (085312b re #139345 (comment)) has been since been pushed.

wesleywiser · 2025-06-26T14:09:30Z

Hi @fmease, it looks like the author has responded to all of your review feedback so this is ready for another look when you get a chance. Thanks!

wesleywiser · 2025-07-31T14:17:44Z

r? rust-lang/compiler

compiler/rustc_interface/src/interface.rs

compiler/rustc_resolve/src/late.rs

lcnr · 2025-08-07T10:53:36Z

compiler/rustc_lint/src/internal.rs

-use rustc_middle::ty::{self, GenericArgsRef, Ty as MiddleTy};
+use rustc_hir::{Expr, ExprKind, HirId};
+use rustc_middle::ty::{
+    self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty as MiddleTy,


Suggested change

self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty as MiddleTy,

self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty,

please avoid importing hir::Ty instead. We generally always uses Ty for ty::Ty

That makes sense to me.

smoelius · 2025-08-14T01:29:54Z

I'm trying to rebase onto master to eliminate the build failure. Please forgive the impending churn.

rustbot · 2025-08-14T21:28:55Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

compiler/rustc_lint/src/internal.rs

lcnr · 2025-08-15T15:08:24Z

lgtm

@bors delegate+

bors · 2025-08-15T15:08:28Z

✌️ @smoelius, you can now approve this pull request!

If @lcnr told you to "r=me" after making some further change, please make that change, then do @bors r=@lcnr

lcnr · 2025-08-15T15:08:41Z

please squash your commits though :3

Fix adjacent code Fix duplicate warning; merge test into `tests/ui-fulldeps/internal-lints` Use `rustc_middle::ty::FnSig::inputs` Address two review comments - rust-lang#139345 (comment) - rust-lang#139345 (comment) Use `Instance::try_resolve` Import `rustc_middle::ty::Ty` as `Ty` rather than `MiddleTy` Simplify predicate handling Add more `#[allow(rustc::potential_query_instability)]` following rebase Remove two `#[allow(rustc::potential_query_instability)]` following rebase Address review comment Update compiler/rustc_lint/src/internal.rs Co-authored-by: lcnr <[email protected]>

smoelius · 2025-08-15T17:57:13Z

@bors r=@lcnr

bors · 2025-08-15T17:57:16Z

📌 Commit c3c2c23 has been approved by lcnr

It is now in the queue for this repository.

Extend `QueryStability` to handle `IntoIterator` implementations This PR extends the `rustc::potential_query_instability` lint to check values passed as `IntoIterator` implementations. Full disclosure: I want the lint to warn about this line (please see rust-lang#138871 for why): https://github.com/rust-lang/rust/blob/aa8f0fd7163a2f23aa958faed30c9c2b77b934a5/src/librustdoc/json/mod.rs#L261 However, the lint warns about several other lines as well. Final note: the functions `get_callee_generic_args_and_args` and `get_input_traits_and_projections` were copied directly from [Clippy's source code](https://github.com/rust-lang/rust/blob/4fd8c04da0674af2c51310c9982370bfadfa1b98/src/tools/clippy/clippy_lints/src/methods/unnecessary_to_owned.rs#L445-L496).

Rollup of 11 pull requests Successful merges: - #139345 (Extend `QueryStability` to handle `IntoIterator` implementations) - #144838 (Fix outdated doc comment) - #145206 (Port `#[custom_mir(..)]` to the new attribute system) - #145208 (Implement declarative (`macro_rules!`) derive macros (RFC 3698)) - #145309 (Fix `-Zregparm` for LLVM builtins) - #145355 (Add codegen test for issue 122734) - #145420 (cg_llvm: Use LLVM-C bindings for `LLVMSetTailCallKind`, `LLVMGetTypeKind`) - #145451 (Add static glibc to the nix dev shell) - #145460 (Speedup `copy_src_dirs` in bootstrap) - #145476 (Fix typo in doc for library/std/src/fs.rs#set_permissions) - #145485 (Fix deprecation attributes on foreign statics) r? `@ghost` `@rustbot` modify labels: rollup

Zalathar · 2025-08-17T02:37:13Z

Failed in rollup: #145488 (comment)

This might need a re-bless.

@bors r-

smoelius · 2025-08-17T11:05:51Z

@lcnr The failure occurs in this test:

Testing stage1 compiletest suite=ui-fulldeps mode=ui (stage0 -> stage1, x86_64-unknown-linux-gnu)

This suggests to me that the stage 1 compiler is failing because it doesn't have the lint enhancement.

If that's correct, is there something I need to do to prevent the stage 1 compiler from running that test?

EDIT: Also, I can reproduce the failure locally. I had been running with --stage 2 and it didn't occur to me to run without. Apologies.

Zalathar · 2025-08-17T11:28:28Z

Because this test is really testing compiler behaviour, and is only in ui-fulldeps because it happens to need rustc_data_structures, running it in “stage 1” (which for ui-fulldeps uses the stage 0 compiler) is questionable.

Therefore, I think it might be appropriate to add an //@ ignore-stage1 directive so that it only runs in stage 2.

smoelius · 2025-08-17T11:37:20Z

Therefore, I think it might be appropriate to add an //@ ignore-stage1 directive so that it only runs in stage 2.

I added that to tests/ui-fulldeps/internal-lints/query_stability.rs and I'm testing it locally now.

smoelius · 2025-08-17T18:55:32Z

I'm experiencing some weirdness with the rustdoc-scrape-examples-paths test, both with and without --stage 2. But I suspect the problem is unrelated to these changes.

I don't want to abuse delegate privileges. So could I trouble you to give this another look, @lcnr?

rustbot assigned fmease Apr 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 4, 2025

This comment has been minimized.

Sign in to view

smoelius mentioned this pull request Apr 4, 2025

Change one FxHashMap to FxIndexMap in librustdoc #138871

Draft

1 task

smoelius force-pushed the into-iter-stability branch from 7c51ef9 to 1a2c5a3 Compare April 4, 2025 02:03

This comment has been minimized.

Sign in to view

smoelius marked this pull request as draft April 4, 2025 10:36

smoelius marked this pull request as ready for review April 4, 2025 21:27

fmease reviewed May 27, 2025

View reviewed changes

smoelius added a commit to smoelius/rust that referenced this pull request May 28, 2025

Address two review comments

2a4d30f

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

This comment has been minimized.

Sign in to view

smoelius added a commit to smoelius/rust that referenced this pull request May 28, 2025

Address two review comments

ae437dd

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

smoelius force-pushed the into-iter-stability branch from 2a4d30f to ae437dd Compare May 28, 2025 22:21

This comment has been minimized.

Sign in to view

rustbot assigned lcnr and unassigned fmease Jul 31, 2025

lcnr reviewed Aug 7, 2025

View reviewed changes

compiler/rustc_interface/src/interface.rs Show resolved Hide resolved

lcnr reviewed Aug 7, 2025

View reviewed changes

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved

lcnr reviewed Aug 7, 2025

View reviewed changes

smoelius force-pushed the into-iter-stability branch 2 times, most recently from 610cb4d to cf80434 Compare August 14, 2025 00:56

This comment has been minimized.

Sign in to view

smoelius force-pushed the into-iter-stability branch from cf80434 to ba9c93e Compare August 14, 2025 21:28

lcnr reviewed Aug 15, 2025

View reviewed changes

compiler/rustc_lint/src/internal.rs Show resolved Hide resolved

smoelius force-pushed the into-iter-stability branch from 49e13a7 to c3c2c23 Compare August 15, 2025 16:11

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2025

Zalathar mentioned this pull request Aug 16, 2025

Rollup of 11 pull requests #145488

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2025

This comment has been minimized.

Sign in to view

Add //@ ignore-stage1 to query_stability.rs test

3b0ff9c

smoelius force-pushed the into-iter-stability branch from 7265541 to 3b0ff9c Compare August 17, 2025 14:32

	self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty as MiddleTy,
	self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty,

Extend QueryStability to handle IntoIterator implementations #139345

Are you sure you want to change the base?

Extend QueryStability to handle IntoIterator implementations #139345

Conversation

smoelius commented Apr 4, 2025

Uh oh!

rustbot commented Apr 4, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

smoelius commented Apr 4, 2025

Uh oh!

fmease commented Apr 4, 2025

Uh oh!

smoelius commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smoelius commented Apr 30, 2025

Uh oh!

smoelius commented May 27, 2025

Uh oh!

fmease left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmease May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmease May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoelius May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

smoelius commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesleywiser commented Jun 26, 2025

Uh oh!

wesleywiser commented Jul 31, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

smoelius commented Aug 14, 2025

Uh oh!

rustbot commented Aug 14, 2025

Uh oh!

Uh oh!

lcnr commented Aug 15, 2025

Uh oh!

bors commented Aug 15, 2025

Uh oh!

lcnr commented Aug 15, 2025

Extend `QueryStability` to handle `IntoIterator` implementations #139345

Extend `QueryStability` to handle `IntoIterator` implementations #139345

smoelius commented Apr 4, 2025 •

edited

Loading

fmease left a comment •

edited

Loading

fmease May 27, 2025 •

edited

Loading

fmease May 27, 2025 •

edited

Loading

smoelius May 28, 2025 •

edited

Loading

smoelius commented May 28, 2025 •

edited

Loading

smoelius commented Aug 17, 2025 •

edited

Loading